Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add experimental 'Details' widget for builds #10147

Merged
merged 42 commits into from
Feb 17, 2025

Conversation

janfaracik
Copy link
Contributor

@janfaracik janfaracik commented Jan 13, 2025

Behind the experimental flag, new-build-page.flag, this PR adds a new 'Details' widget to build pages. The widget lists information about the build (currently just a small set for this PR), which makes it easy to see at a glance what caused the build, how long it took, and any relevant SCM information.

This is just a subset of the final information that could go into this widget, intended to keep the PR small and reviewable.

We have a new extensible API too, which allows plugins, such as the Git Branch Source plugin, to hook in to the Details widget to contribute its own Detail items.

image

Downstream PR here jenkinsci/github-branch-source-plugin#826 (Need to move the detail to scm-api will be done soon)


An example of what a more complete implementation of this could look like is seen in the Pipeline Graph View plugin:

image

Testing done

  • Details show as expected when the flag is enabled
  • No changes to the interface when the flag is off

Proposed changelog entries

  • Add experimental 'Details' widget for builds

Proposed upgrade guidelines

N/A

Submitter checklist

Preview Give feedback

Desired reviewers

@jenkinsci/sig-ux

Before the changes are marked as ready-for-merge:

Maintainer checklist

Preview Give feedback

@janfaracik janfaracik requested a review from a team January 13, 2025 09:41
@janfaracik janfaracik added web-ui The PR includes WebUI changes which may need special expertise major-rfe For changelog: Major enhancement. Will be highlighted on the top labels Jan 13, 2025
@timja timja added the needs-security-review Awaiting review by a security team member label Jan 13, 2025
@timja timja requested a review from a team January 13, 2025 09:49
core/src/main/java/jenkins/model/Detail.java Outdated Show resolved Hide resolved
/**
* Returns true if this detail is applicable to the given Actionable object
*/
public boolean isApplicable() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Action this would be the job of the factory. Why create an object just to discard it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair, I was using isApplicable to encapsulate show/hide logic of the detail inside of the Detail, so that its factory didn't need to worry about that.

Comment on lines 18 to 20
public static DetailGroup SCM = new DetailGroup(0);

public static DetailGroup GENERAL = new DetailGroup(Integer.MAX_VALUE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these not @Extension(ordinal = …)?

src/main/js/pages/job/index.js Outdated Show resolved Hide resolved
src/main/js/pages/job/index.js Outdated Show resolved Hide resolved
core/src/main/java/hudson/Functions.java Show resolved Hide resolved
details.addAll(taf.createFor(object));
}

Map<DetailGroup, List<Detail>> orderedMap = new TreeMap<>(Comparator.comparingInt(DetailGroup::getOrder));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so weird only because it doesn't use @Extension.


for (Detail detail : details) {
if (detail.isApplicable()) {
orderedMap.computeIfAbsent(detail.getGroup(), k -> new ArrayList<>()).add(detail);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not extensible if it doesn't handle getOrder clashes gracefully.

@uhafner
Copy link
Member

uhafner commented Jan 13, 2025

  • Since the current summary is an non-configurable mess I wonder how we can make it better in the details widget now?
  • What is the difference between the new details widget and the summary widget?
  • How can we enforce that the UI of a detail element is just a line with a text and icon? Otherwise we get the same thing as in the summary...
  • Is there a configuration planned? (Order, Visibility)

@timja
Copy link
Member

timja commented Jan 13, 2025

  • Since the current summary is an non-configurable mess I wonder how we can make it better in the details widget now?
  • What is the difference between the new details widget and the summary widget?

Summary widget is basically 'Uncategorised' from the Manage Jenkins page.
The plan is to add some high level cards and then allow plugins to add their own as well.

I'm thinking for the next core ones:

  • Changes (Jan has something for this I think mostly working)
  • Artifacts

Plugin ones:

  • Test results (potentially in the junit plugin or maybe a test-api plugin)
  • Code coverage
  • Static analysis warnings

Need to do a bit of analysis to see whats going on the current summary pane and needs to be there / should be removed.

e.g. we plan to get rid of all the Git Action data thats displayed there...

  • How can we enforce that the UI of a detail element is just a line with a text and icon? Otherwise we get the same thing as in the summary...

I think for the Details card anything else will look quite silly so potentially it will be fine because of the building blocks supplied.

Its possible we don't even need this pane to be jelly-able. We started out with it as Jelly but Jan's moved I think most of it to be done in Java now. I think the date format one is the only thing left in Jelly which could potentially be handled in another way if its worth it.

  • Is there a configuration planned? (Order, Visibility)

There's an order through code in this.
By configuration do you mean user configuration or extension configuration?

Potentially, we need to review more of what goes on the summary page.

@uhafner
Copy link
Member

uhafner commented Jan 14, 2025

  • Is there a configuration planned? (Order, Visibility)

There's an order through code in this. By configuration do you mean user configuration or extension configuration?

I don't think that order by code will be sufficient. This will lead to the same problem as we have right now. Before we are developing new widgets we should better spend some time on how this can be configured by users. Like this has been done by e.g. Apple on IOS devices (and now on the desktop as well): first of all, they provided a fixed grid (tiles) and a user configuration of this grid. Users can change the size of the widgets (in terms of tiles), order of widgets, visibility of widgets. Not until they released this concept they added widgets step by step... Otherwise we get the same thing as now: too many widgets compete for the best place in the small visible area.

Co-authored-by: Tim Jacomb <21194782+timja@users.noreply.github.com>
@janfaracik
Copy link
Contributor Author

I don't think that order by code will be sufficient. This will lead to the same problem as we have right now. Before we are developing new widgets we should better spend some time on how this can be configured by users. Like this has been done by e.g. Apple on IOS devices (and now on the desktop as well): first of all, they provided a fixed grid (tiles) and a user configuration of this grid. Users can change the size of the widgets (in terms of tiles), order of widgets, visibility of widgets. Not until they released this concept they added widgets step by step... Otherwise we get the same thing as now: too many widgets compete for the best place in the small visible area.

Heya. Thanks for the feedback - I do agree, I think end goal is definitely a significant increase in structured customisability from users (and plugin developers) of what the UI can do. Keen to avoid the issues we have now where it's essentially a free-for-all for developers to put what they want where, some hierarchy (without being too restrictive) is definitely needed.

What I'd like to do with these experiments is prove out our current thinkings with real world users, e.g. is the 'Details' widget a sensible place for build/SCM info, is it more organised than what we have now, is this the best way to present this information.

Right now, though, this is just an experiment. It’s simple to implement and easy to roll back if it doesn’t work out, but I'm hopeful it's a positive addition and users respond well. Looking to blog about it/share it with the community once it's live for feedback/polls.

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, especially since its all behind an experimental flag anyway so we can iterate this more as needed.

@timja timja requested a review from a team February 14, 2025 18:48
@krisstern
Copy link
Member

We are seeing an error in the log as below:

Could not retrieve XPath >//button[contains(@class, 'jenkins-submit-button')]< on HtmlForm[<form method="post" autocomplete="off" name="parameters" action="build?delay=0sec" class="jenkins-form">]

Copy link
Member

@krisstern krisstern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@krisstern
Copy link
Member

/label ready-for-merge

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Feb 16, 2025
@krisstern krisstern merged commit eab7102 into jenkinsci:master Feb 17, 2025
16 checks passed
@janfaracik janfaracik deleted the experimental-details-widget branch February 17, 2025 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-rfe For changelog: Major enhancement. Will be highlighted on the top ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants